-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Put into cache using root entity name #8009
Conversation
foreach ($result as $index => $entity) { | ||
$identifier = $this->uow->getEntityIdentifier($entity); | ||
$entityKey = new EntityCacheKey($entityName, $identifier); | ||
$entityKey = new EntityCacheKey($cm->getMetadataValue('rootEntityName'), $identifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you call $cm->getMetadataValue('rootEntityName')
inside the loop?
Can you please also put the =
in line? Probably removing spaces of $identifier
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first I called the property of the metadata directly and then changed to the method call in situ, hence remaining inside the loop. I see your point though, no need to call each time. Updated now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed the alignment too.
|
||
class DDC7969Test extends SecondLevelCacheAbstractTest | ||
{ | ||
public function testChildEntityRetrievedFromCache() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public function testChildEntityRetrievedFromCache() | |
public function testChildEntityRetrievedFromCache() : void |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated now.
Is there anything I can do to trigger the travis build again without updating the PR? I don't think the failure is related to anything I added. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to have more reviewers from the Doctrine team accepting this PR in case I overlooked a detail in the functionality.
No problem, do I need to add more reviewers? |
$identifier = $this->uow->getEntityIdentifier($entity); | ||
$entityKey = new EntityCacheKey($entityName, $identifier); | ||
$identifier = $this->uow->getEntityIdentifier($entity); | ||
$entityKey = new EntityCacheKey($rootEntityName, $identifier); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$entityKey = new EntityCacheKey($rootEntityName, $identifier); | |
$entityKey = new EntityCacheKey($cm->rootEntityName, $identifier); |
@@ -279,9 +279,12 @@ public function put(QueryCacheKey $key, ResultSetMapping $rsm, $result, array $h | |||
|
|||
$region = $persister->getCacheRegion(); | |||
|
|||
$cm = $this->em->getClassMetadata($entityName); | |||
$rootEntityName = $cm->getMetadataValue('rootEntityName'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
$rootEntityName = $cm->getMetadataValue('rootEntityName'); |
You should directly use $cm->rootEntityName
in line 287, getMetadataValue
has a different use-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @beberlei , I originally did that but then the codesniffer job failed due to accessing a property when presented only with an interface - should we suppress or ignore that?
Actually looking at the report again, I could add the instance of check but that seems a bit clunky still:
IssueId: 42927595 Message: Accessing
rootEntityName
on the interfaceDoctrine\Common\Persistence\Mapping\ClassMetadata
suggest that you code against a concrete implementation. How about adding aninstanceof
check?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@peterkeatingie yes, you can add assert($cm instanceof ClassMetadata);
in line 283, this code gets compiled away in production when assertions are off.
Thanks @beberlei @SenseException. Is there anything I can do expedite this PR or just wait now? |
@peterkeatingie i am waiting from someone with more SLC experience to ack. One question I have though, you made this PR towards 2.8.x branch, but I would consider this a bugfix, so this would be good for the 2.7 branch. What was your approach here? |
No problem. Actually I might have this wrong - I thought our project had 2.8 using composer show but maybe got that wrong or maybe that's for dev, I can't remember. Anyway checked the master lock file now and it's 2.7 alright. Also was following this advice: https://github.com/doctrine/orm#which-branch-should-i-choose. So if that should be against 2.7 then I guess I need to branch that and make a new PR?! |
Closing this in favour of #8023 |
The cache key for reading is created based on the entity root name but the cache was written with a key based on the found entity (the child in this case) resulting in cache miss.
Using the same root name when putting fixes the issue. I do believe that any extending instances should have unique identifiers as they will have to share an identifier from the base class. This will mean the key won't be ambiguous - if my thinking is correct.
#7969